Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow user to specify 'step' key while logging metrics #808

Merged
merged 6 commits into from
Feb 16, 2020
Merged

Allow user to specify 'step' key while logging metrics #808

merged 6 commits into from
Feb 16, 2020

Conversation

festeh
Copy link
Contributor

@festeh festeh commented Feb 9, 2020

What does this PR do?

Fixes #788. Wasn't discussed, but there's some interest in this feature, so I decided to try.

@Borda Borda added the feature Is an improvement or enhancement label Feb 10, 2020
@Borda Borda added this to the 0.6.1 milestone Feb 10, 2020
@Ir1d
Copy link
Contributor

Ir1d commented Feb 10, 2020

LGTM @festeh , can you add some tests for this key?

@festeh
Copy link
Contributor Author

festeh commented Feb 11, 2020

@Ir1d added test. It's somewhat involved, let me know if it could be made simpler or something else should be also tested.

@williamFalcon
Copy link
Contributor

@festeh i don't understand this feature. Why can't a user already specify step?

@festeh
Copy link
Contributor Author

festeh commented Feb 12, 2020 •
edited
Loading

@williamFalcon Technically she can by manually casting tensor values to scalars and pass them to self.logger.log_metrics which supports this argument. I didn't notice a simpler way, so I proposed this feature. Is there a simpler approach?

Borda
Borda requested changes Feb 14, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work, could you have look at my comments... :]

pytorch_lightning/trainer/logging.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/logging.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/logging.py Show resolved Hide resolved
pytorch_lightning/trainer/logging.py Show resolved Hide resolved
tests/test_logging.py Outdated Show resolved Hide resolved
Borda
Borda reviewed Feb 14, 2020
View reviewed changes
tests/test_logging.py Outdated Show resolved Hide resolved
Borda
Borda approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, Thx :]

@Borda Borda added the ready PRs ready to be merged label Feb 14, 2020
@williamFalcon williamFalcon merged commit 06ca642 into Lightning-AI:master Feb 16, 2020
hanbyul-kim pushed a commit to hanbyul-kim/pytorch-lightning that referenced this pull request Feb 16, 2020
@festeh @hanbyul-kim
Allow user to specify 'step' key while logging metrics (Lightning-AI#808
bb1d707 
)

* allow to specify 'step' key

* add test

* docs to log_metrics

* fix test

* rename

* also rename
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

Assignees
No one assigned
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.7
Development

Successfully merging this pull request may close these issues.

Ability to specify step for logged metrics
4 participants
@festeh @Ir1d @williamFalcon @Borda